respect rate limits during pagination by sleeping #167

jmanian · 2017-09-18T17:03:37Z

resolves #166

Here's an initial attempt at respecting rate limits during pagination. No tests here yet.

dblock · 2017-09-18T17:09:01Z

This is a good start, it needs some tests, CHANGELOG and such.

Maybe we can also hint a sleep value in the options passed into the function and avoid TooManyRequestsError in most cases?

jmanian · 2017-09-18T17:22:26Z

That's a good idea about hinting a sleep value, though we may find it hard to tune to avoid the errors. The reason I say this is I asked some Slack folks about their rate limiting a while back and they had this to say:

Our rate limiting is pretty brute. At the same time, we don't consider getting rate limited a ding or demerit. Some devs might just want to burn through as fast as possible until hitting a rate limit, then pausing activity before resuming, theoretically, after following the Retry-after. (source)

The specific rate limits for the web API aren't actually published and can vary somewhat method to method, with some burst behavior allowed here and there. By virtue of being unpublished, the best recommendation is to make requests until you hit a limit, then backoff until the time the retry-after indicates. (source)

Given this context, I think the best solution is to allow the individual dev to pass in their own default sleep time, so that they can tune to their use case, but not have any default sleep behavior beyond the rate limiting.

dblock · 2017-09-18T17:43:54Z

I agree, lets default the hint to zero.

I wonder whether retry behavior could be optionally built into everything? As a client I just don't want to deal with it :) But that can be a later PR/feature - feel free to open an issue for that too.

jmanian · 2017-09-18T19:14:13Z

I think this is what you had in mind for hinting a sleep value. I went with pause for the parameter so as to not overload sleep with an attribute on top of the method, but I'm happy to change this.

dblock

Getting much closer, hang on in there. Something should also be added in README around pagination about this, at least to document the new pause option(s).

dblock · 2017-09-18T21:09:52Z

CHANGELOG.md

@@ -1,5 +1,6 @@
 ### 0.9.2 (Next)

+* [#167](https://github.com/slack-ruby/slack-ruby-client/pull/167): Respect rate limits during pagination by sleeping. Also add optional `pause` parameter in order to proactively sleep between each paginated request - [@jmanian](https://github.com/jmanian).


Maybe simply "Added support for pausing between paginated requests that cause Slack rate limiting"?

dblock · 2017-09-18T21:11:37Z

lib/slack/web/pagination/cursor.rb

@@ -7,23 +7,31 @@ class Cursor

          attr_reader :client
          attr_reader :verb
+          attr_reader :pause


I feel a bit uncomfortable calling this pause because it's unclear whether that's a verb or a noun, maybe sleep_interval? Don't feel strongly about this one though.

dblock · 2017-09-18T21:12:41Z

lib/slack/web/pagination/cursor.rb

+                response = client.send(verb, query)
+              rescue Slack::Web::Api::Errors::TooManyRequestsError => e
+                sleep(e.retry_after.seconds)
+                next


I also think we should have a max retry count with some default set fairly high. If slack blacklists you somehow you'll have code that never ends and it will be impossible to debug.

I also think there should be a log line here in DEBUG mode.

Do you think the count should reset after a successful request?

I think so.

dblock

Last problem with params, then I'm merging. Thanks!

dblock · 2017-09-19T01:13:34Z

lib/slack/web/pagination/cursor.rb

          attr_reader :params

          def initialize(client, verb, params = {})
            @client = client
            @verb = verb
+            @sleep_interval = params.delete(:sleep_interval)


This modifies incoming params, ie. has a side-effect, you don't want to do that, so just params[:sleep_interval].

dblock · 2017-09-19T01:16:11Z

lib/slack/web/pagination/cursor.rb

+                retry_count += 1
+                sleep(e.retry_after.seconds)
+                next
+              end
              yield response


You can move this inside the rescue block and avoid next, right?

Also did I tell you that I was a nitpicky code reviewer? Hang on tight :)

I understand, I'm the same way 😎.

Which part are you suggesting go in the rescue block? I started out with everything inside the begin block (see first commit b75bfcb) which avoids the next. Is this what you mean?

I changed it (8542d00) because I thought it was bad practice to have more lines than necessary inside the begin block — even if just for clarity (so that it's clear which line is expected to raise the error). But in this case it's obvious where the error is coming from, so I guess it's fine either way.

I actually now think yours is better. Someone can do something inside the yield that causes that exception and we're going to have bad side effects and catching an error we shouldn't.

dblock · 2017-09-19T13:46:42Z

Merged. Thanks!

dblock · 2017-09-19T13:51:55Z

I made a small change to dup the params and use delete in cdb697b.

jmanian · 2017-09-19T14:28:26Z

I was debating just leaving those parameters in there, because I think all the API methods ignore those extraneous parameters. But it felt more right to remove them.

dblock · 2017-09-19T17:47:25Z

I opened #168 - interested in giving that one a try?

icybin · 2017-09-20T07:21:42Z

@dblock Is this problem related to login/authentication stage? I noticed that when I deployed the bot (ruby-slack-bot) on my laptpo, the bot could be connected to Slack Api. (well, may laptop network is slow)

However, on AWS ec2 where the network is extremely fast, the first 2 requests to Slack happen in within 1 second (I can see this when I turn on DEBUG mode), and then Slack simply blocks the future 3rd request. The bot will never be able to authenticate to the service.

dblock · 2017-09-20T11:39:15Z

@kyanh-huynh #168 would mitigate that as well.

respect rate limits during pagination by sleeping

b75bfcb

only rescue from the call

8542d00

jmanian added 2 commits September 18, 2017 15:09

add pause parameter for pagination

791e00f

add tests

e041d16

add to changelog

812e78f

jmanian force-pushed the pagination_rate_limiting branch from 16dc964 to 812e78f Compare September 18, 2017 19:47

dblock requested changes Sep 18, 2017

View reviewed changes

jmanian added 3 commits September 18, 2017 17:25

rename parameter to sleep_interval

1e44ccc

don't retry after hitting max retries

c8e11eb

update readme and changelog

549563b

jmanian force-pushed the pagination_rate_limiting branch from 3406f4a to 549563b Compare September 18, 2017 22:36

dblock requested changes Sep 19, 2017

View reviewed changes

dblock reviewed Sep 19, 2017

View reviewed changes

don't modify incoming params

f348110

dblock merged commit 201c751 into slack-ruby:master Sep 19, 2017

dblock mentioned this pull request Sep 19, 2017

Automatically retry calls on 429 rate limiting #168

Open

jmanian deleted the pagination_rate_limiting branch September 19, 2017 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

respect rate limits during pagination by sleeping #167

respect rate limits during pagination by sleeping #167

jmanian commented Sep 18, 2017

dblock commented Sep 18, 2017

jmanian commented Sep 18, 2017

dblock commented Sep 18, 2017

jmanian commented Sep 18, 2017

dblock left a comment

dblock Sep 18, 2017

dblock Sep 18, 2017

dblock Sep 18, 2017

jmanian Sep 18, 2017

dblock Sep 18, 2017

dblock left a comment

dblock Sep 19, 2017

dblock Sep 19, 2017

dblock Sep 19, 2017

jmanian Sep 19, 2017

dblock Sep 19, 2017

dblock Sep 19, 2017

dblock commented Sep 19, 2017

dblock commented Sep 19, 2017

jmanian commented Sep 19, 2017

dblock commented Sep 19, 2017

icybin commented Sep 20, 2017 •

edited

Loading

dblock commented Sep 20, 2017 •

edited

Loading

		@@ -1,5 +1,6 @@
		### 0.9.2 (Next)

		* [#167](https://github.com/slack-ruby/slack-ruby-client/pull/167): Respect rate limits during pagination by sleeping. Also add optional `pause` parameter in order to proactively sleep between each paginated request - [@jmanian](https://github.com/jmanian).

respect rate limits during pagination by sleeping #167

respect rate limits during pagination by sleeping #167

Conversation

jmanian commented Sep 18, 2017

dblock commented Sep 18, 2017

jmanian commented Sep 18, 2017

dblock commented Sep 18, 2017

jmanian commented Sep 18, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Sep 19, 2017

dblock commented Sep 19, 2017

jmanian commented Sep 19, 2017

dblock commented Sep 19, 2017

icybin commented Sep 20, 2017 • edited Loading

dblock commented Sep 20, 2017 • edited Loading

icybin commented Sep 20, 2017 •

edited

Loading

dblock commented Sep 20, 2017 •

edited

Loading